feat: launch one remote workspace for all workspace folders #1348
Conversation
LiGaCu
changed the title
| private workspaceFolders: WorkspaceFolder[] | ||
| public dependencyHandlerRegistry: LanguageDependencyHandler<BaseDependencyInfo>[] = [] | ||
| private initializedWorkspaceFolder = new Map<WorkspaceFolder, boolean>() | ||
| // Create a SharedArrayBuffer with 4 bytes (for a 32-bit unsigned integer) for thread-safe counter |
There was a problem hiding this comment.
Just for posterity, I believe we don't need to go with this approach since we are not using any workers or threads in the code and everything is single threaded at the moment
a simple data type like the following would also work here
type DependencySizeTracker = {
totalSize: number
maxSize: number
}
const sizeTracker: DependencySizeTracker = {
totalSize: 0,
maxSize: 8 * 1024 * 1024 * 1024
}
The current approach is still fine and I am not suggesting we change it.
| dispose(): void { | ||
| this.dependencyMap.clear() | ||
| this.dependencyUploadedSize.clear() | ||
| Atomics.store(this.dependencyUploadedSizeSum, 0, 0) |
There was a problem hiding this comment.
This should be done once inside the dispose function of the dependencyDiscoverer instead of being done inside each individual dependency handler multiple times
There was a problem hiding this comment.
Make sense, will move it.
| }, this.CONTINUOUS_MONITOR_INTERVAL) | ||
| // Otherwise, wait for the promise to resolve or catch the rejection and retry | ||
| try { | ||
| return await this.remoteWorkspaceIdPromise |
There was a problem hiding this comment.
What happens when there is a client side issue like user credentials expired or some backend problem, wouldn't this promise just keep getting rejected over and over and wouldn't it cause a continuous loop of waitForRemoteWorkspaceId being called with no exit
Is there some other exit scenario here that I am maybe missing?
There was a problem hiding this comment.
The waitForRemoteWorkspaceId function is currently being called inside an interval right now from initializeWorkspaceStatusMonitor
If the waitForRemoteWorkspaceId call goes into an infinite cycle, new intervals would start getting created before the previous one resolves. So the number of active intervals would linearly increase since none of the open intervals would resolve. This could lead to very unpleasant UX for the unhappy paths
Edit: I misread the code so the first part of this comment is wrong and should be ignored.
The function waits until the promise resolves (and it can possibly never resolve because we never get a workspace id) so it should be used carefully, but the suggested usages in this PR look good to me. In the future we can consider giving up on waiting the promises after a certain period of time has passed as an improvement
There was a problem hiding this comment.
The waitForRemoteWorkspaceId function is currently being called inside an interval right now from initializeWorkspaceStatusMonitor
Not really, neither initializeWorkspaceStatusMonitor nor checkRemoteWorkspaceStatusAndReact would call waitForRemoteWorkspaceId. So, none of these intervals would get blocked by remoteWorkspaceIdPromise and no loop would be formed.
| this.workspaceState.webSocketClient | ||
| .send(message) | ||
| .then(() => { | ||
| this.logging.log(` Message sent successfully`) |
There was a problem hiding this comment.
styling: unnecessary space before the log
There was a problem hiding this comment.
also the log is somewhat ambiguous, would be helpful to at least specify that this is a websocket message
There was a problem hiding this comment.
Sure, will modify this log.
| }) | ||
| } | ||
| } | ||
| }, this.MESSAGE_PUBLISH_INTERVAL) |
There was a problem hiding this comment.
Given we now have the websocket message frequency being controlled by the workspace folder manager, I believe we should be able to simplify and remove the wrong throttling that we have on the websocket client
We should remove this line -> https://github.com/aws/language-servers/blob/main/server/aws-lsp-codewhisperer/src/language-server/workspaceContext/client.ts#L159 and make the send function synchronous. It will also help simplify rest of the code where send was being called
Line 159 linked above is not useful even now since it applies a 100 ms delay to every websocket send call. So if send is called with 5 different messages at the same time, all the calls will wait 100 ms and they will all be sent at the same time meaning that the existing delay is actually completely redundant.
The scenario explained in the comment https://github.com/aws/language-servers/blob/main/server/aws-lsp-codewhisperer/src/language-server/workspaceContext/client.ts#L144-L156 is too extreme of an example and as explained in the paragraph above, the artificial delay is redundant
There was a problem hiding this comment.
Make sense, will remove the wait within WebSocketClient
| this.logging.error(`Error sending didChangeWorkspaceFolders message: ${e}`) | ||
| }) | ||
| } | ||
| websocketClient.disconnect() |
There was a problem hiding this comment.
is there any case where we would want to disconnect the websocket client to be more efficient with resource usage? For example if the user removes all workspace folders from their workspace. If the user adds a new workspace folder after this happens, the code should probably be able to re-open a new ws connection
There was a problem hiding this comment.
yeah, those are the improvements we could consider separately.
| workspaceFolders: WorkspaceFolder[], | ||
| credentialsProvider: CredentialsProvider | ||
| credentialsProvider: CredentialsProvider, | ||
| workspaceIdentifier: string |
There was a problem hiding this comment.
As a followup we should also update the commented out logic for inline completions so that inline completions server will also be ready to use the workspaceIdentifier
There was a problem hiding this comment.
Client-side generated workspaceIdentifier are not supposed to be used by others (like inline completions), they would need to fetch the server-side generated workspaceState.workspaceId and use that in API requests.
I would say we can add an interface separately from this PR.
| }, | ||
| }) | ||
|
|
||
| // We add this event to the front of the queue here to prevent any race condition that might put events before the didChangeWorkspaceFolders event |
There was a problem hiding this comment.
this comment can be removed from here now that we are no longer adding the message to the front of the queue anymore. This was a best effort mechanism to decrease the likelihood of sending other messages before the didChangeWorkspaceFolders message but it is not a strong mechanism.
As far as I can see it is still possible that if the ws connection gets established before than the source code preparation is finished, any LSP event like didSave event can be sent before the didChangeWorkspaceFolders event.
Maybe we should add a flag to the websocket publishing logic in workspace folder manager to keep track of the initial didChangeWorkspaceFolders being sent and until the initial didChangeWorkspaceFolders event(s) is/are sent, the websocket messages would be queued. Not suggesting to do it in this PR but would be a nice followup
## Problem In order to coordinate the new aws/language-servers#1348 change, GenerateCompletions requests would start expecting the `fileUri` field to be set. ## Solution Add `fileUri` to FileContext --- - Treat all work as PUBLIC. Private `feature/x` branches will not be squash-merged at release time. - Your code changes must meet the guidelines in [CONTRIBUTING.md](https://github.com/aws/aws-toolkit-vscode/blob/master/CONTRIBUTING.md#guidelines). - License: I confirm that my contribution is made under the terms of the Apache 2.0 license. --------- Co-authored-by: Jiatong Li <[email protected]>
| if (this.ws?.readyState === WebSocket.OPEN) { | ||
| await new Promise(resolve => setTimeout(resolve, this.messageThrottleDelay)) | ||
| this.ws.send(message) | ||
| this.logging.debug('Message sent successfully to the remote workspace') |
There was a problem hiding this comment.
reminder that debug logs are disabled by default on production
There was a problem hiding this comment.
yeah, not seeing these messages valuable to present in user logs.
3 participants
Problem
For server-side workspace context capability, we would like to consolidate all the workspace folders under the same IDE workspace into a single remote workspace so that we could provide better workspace-level context for cross-workspace-folder scenarios.
Prerequisite changes have been made:
Solution
Updated the fundamental logic of
workspaceContext/workspaceFolderManagerto map all the events into a single remote workspace. ModifiedworkspaceContext/workspaceContextServerlogic to accept the workspace identifier passed from extensions before kicking off the workflow. ModifiedDependencyDiscoverer,LanguageDependencyHandler,ArtifactManager, etc. to accommodate the logic updates.Performed E2E testing and verified the logic works well.
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.